Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement System.Runtime.InteropServices.CLong/CULong/NFloat #46401

Merged
merged 23 commits into from
Jan 12, 2021
Merged

Implement System.Runtime.InteropServices.CLong/CULong/NFloat #46401

merged 23 commits into from
Jan 12, 2021

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Dec 25, 2020 •
edited
Loading

Fixes #13788

Implements the System.Runtime.InteropServices.CLong, CULong and, NFloat types along with code to recognize them as JIT intrinsics in the unmanaged calling convention.

Includes framework tests for the APIs and updates to the ThisCall runtime test to validate the special JIT behavior.

cc: @dotnet/jit-contrib for the JIT changes.

@jkoritzinsky jkoritzinsky added this to the 6.0.0 milestone Dec 25, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

AaronRobinsonMSFT
AaronRobinsonMSFT reviewed Dec 26, 2020
View reviewed changes
src/coreclr/jit/compiler.cpp Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/CLong.cs

namespace System.Runtime.InteropServices
{
[CLSCompliant(false)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means this type isn't compliant in VB.NET. Which means APIs that decide to use it will not be consumable by VB.NET. Unsure if that is okay or not. I am fine with it but let's make sure we do our due diligence and confirm that shared understanding.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think CLong can be CLSCompliant since it is signed and internally an int or nint, which both are CLSCompliant. However, I don't think CLSCompliant means it isn't consumable by VB.NET, using other types like UIntPtr or SByte seem to work just fine: https://sharplab.io/#v2:DYLgbgRgNALiCWwA+BJAtgBwPYCcYGcACAZQE98YBTNAWACgAFAVwmHgGNCBhYAQ3yJd6hEYWasOhAGJMAduxjwsswgFkAjAAoAlIQCCRAKopZMBjBzDR1gEqUYTHCuOnzOAHQAtSjixWRAKKyACbScgpKsv6E0eJsnDLyispqAEw6+kTEAEKkVNG29o4qALTq0UGhiRHK9JXcfAJAA=

I'm no expert in VB though, so it's definitely plausible that I'm unaware of some detail 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLSCompilant(false) just means that some or all of the members use types that are not guaranteed to be usable from all languages. The types themselves are still usable, but some members (like the uint constructor on CULong) aren't supported. It doesn't block the types from being used in VB.NET.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything actually preventing the use of this from other languages given it is just a struct wrapping a CLSCompliant type?

src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems Show resolved Hide resolved
src/tests/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallTest.cs Show resolved Hide resolved
sandreenko
sandreenko reviewed Dec 26, 2020
View reviewed changes
src/coreclr/jit/compiler.h
@@ -8028,6 +8033,21 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
#endif
}

bool isIntrinsicType(CORINFO_CLASS_HANDLE clsHnd)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was it just moved or do I miss an actual change for these 3 functions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just moved out of the FEATURE_SIMD block.

src/coreclr/jit/compiler.cpp Show resolved Hide resolved
src/coreclr/jit/compiler.cpp
{
canReturnInRegister = false;
howToReturnStruct = SPK_ByReference;
useType = TYP_UNKNOWN;
}
#elif defined(TARGET_WINDOWS) && !defined(TARGET_ARM)
if (callConvIsInstanceMethodCallConv(callConv))
if (callConvIsInstanceMethodCallConv(callConv) && !isNativePrimitiveStructType(clsHnd))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, it looks like this call is expensive, should we measure TP on x64 windows?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be expensive since its initially filtered on isIntrinsicType, which should just be a simple cached flag check?

@runfoapp runfoapp bot mentioned this pull request Jan 8, 2021
Open
@jkoritzinsky
Copy link
Member Author

@tannergooding any suggestions on how to get the ToString tests passing on the Spanish culture Helix queues?

@tannergooding
Copy link
Member

any suggestions on how to get the ToString tests passing on the Spanish culture Helix queues?

Any reason we can't just say Assert.Equal(rawValue.ToString(), interopValue.ToString())?
That way we aren't relying on the constant value provided lining up with the current culture?

@jkoritzinsky jkoritzinsky mentioned this pull request Jan 11, 2021
Closed
@jkoritzinsky jkoritzinsky mentioned this pull request Jan 11, 2021
Closed
@jkoritzinsky
Copy link
Member Author

Mono failures are due to #46820. I'll disable the ThisCall test on Mono against that issue.

@jkoritzinsky
Copy link
Member Author

Now that CI is green, can I get another review pass on this PR?

tannergooding
tannergooding approved these changes Jan 12, 2021
View reviewed changes
Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

sandreenko
sandreenko approved these changes Jan 12, 2021
View reviewed changes
Copy link
Contributor

@sandreenko sandreenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jit changes LGTM

@jkoritzinsky
Copy link
Member Author

Allconfigurations failure fixed by #46866

@jkoritzinsky jkoritzinsky merged commit 79e0e27 into dotnet:master Jan 12, 2021
@jkoritzinsky jkoritzinsky deleted the clong branch January 12, 2021 23:29
@ghost ghost locked as resolved and limited conversation to collaborators Feb 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas left review comments

@am11 am11 am11 left review comments

@stephentoub stephentoub stephentoub left review comments

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT approved these changes

@sandreenko sandreenko sandreenko approved these changes

@tannergooding tannergooding tannergooding approved these changes

@elinor-fung elinor-fung Awaiting requested review from elinor-fung

Assignees
No one assigned
Labels
area-System.Runtime.InteropServices new-api-needs-documentation
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

Expose CLong, CULong, and NFloat interchange types
8 participants
@jkoritzinsky @Dotnet-GitSync-Bot @tannergooding @stephentoub @am11 @jkotas @sandreenko @AaronRobinsonMSFT